Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use wrapper to allow patching global fetch #377

Merged
merged 6 commits into from
Aug 27, 2024

Conversation

dwightjack
Copy link
Contributor

πŸ”— Linked issue

#295

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

I was investigating the usage of MSW on Nuxt 3 and encountered #295. Looking at the source code, I think that a possible solution is to define the local fetch constant as getter instead of a reference to globalThils.fetch. In this way, 3rd party library can patch and overwrite the global object without us referencing an outdated function:

// Before
export const fetch = _globalThis.fetch

// After
export const fetch = (...args: Parameters<typeof globalThis.fetch>) => _globalThis.fetch(...args)

At the moment, I cannot think of possible breaking changes or side effects caused by this change, but I might miss something.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@rinux55
Copy link

rinux55 commented Mar 21, 2024

nuxt/test-utils#775 just mentioning this one here, thanks for taking this up!

@pi0 pi0 changed the title Refactor: store a local reference of fetch as a wrapper to allow patching fix: use wrapper to allow patching global fetch Mar 21, 2024
@pi0
Copy link
Member

pi0 commented Mar 21, 2024

Thanks for PR. I was thinking of same solution too. I agree it is needed but (sadly) means 1) we allow patching behavior of a standard primitive 2) adds small call overhead.

@dwightjack Have you tried this solution btw? I guess we might need to patch the node entrypoint as well. (also in createFetch we persist the fetch variable)

@pi0 pi0 marked this pull request as draft March 21, 2024 12:43
@dwightjack
Copy link
Contributor Author

Thanks for PR. I was thinking of same solution too. I agree it is needed but (sadly) means 1) we allow patching behavior of a standard primitive 2) adds small call overhead.

  1. Not a fan as well, but I guess it's the only way for them to intercept fetch calls
  2. I am not sure how much this could impact the performances πŸ€” For sure, it's an unneeded change for many use cases.

@dwightjack Have you tried this solution btw? I guess we might need to patch the node entrypoint as well. (also in createFetch we persist the fetch variable)

Oh, I missed that part. I updated the PR (4026829).

I wasn't able to make a full reproduction of Nuxt + MSW on StackBlitz (I guess there are some problems with service workers), so I pushed it to a GitHub repository: https://github.com/dwightjack/ofetch-msw-reproduction

Some notes:

  1. The repository includes the packed version of ofetch built from this PR (I am not sure if there's a more elegant way to do this) and I defined an override for every instance of ofetch.
  2. I implemented MSW in a Nuxt plugin (server + client) and a Nitro plugin (just in case)
  3. Mocking an API call with useFetch seems to work for client-only calls, but fails for server calls
  4. In the node-app folder I ported the small node application linked in the original issue using both the un-modified and the patched versions of ofetch. In that case, the patched version seems to work correctly, so I am not sure why it is not working on Nuxt. πŸ€” Could it be related to also in createFetchwe persist thefetch variable?

Copy link

codecov bot commented Mar 22, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 67.08%. Comparing base (27996d3) to head (63afebd).
Report is 35 commits behind head on main.

Files Patch % Lines
src/index.ts 0.00% 3 Missing ⚠️
src/node.ts 66.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #377       +/-   ##
===========================================
+ Coverage   56.86%   67.08%   +10.22%     
===========================================
  Files          16       16               
  Lines         728      474      -254     
  Branches      113      116        +3     
===========================================
- Hits          414      318       -96     
+ Misses        303      146      -157     
+ Partials       11       10        -1     

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@vanstinator
Copy link

vanstinator commented Mar 23, 2024

I'm working to add some CF Pages support to otel-cf-workers and auto-instrumentation of fetch has no effect there for folks using ofetch, because by the time globalThis.fetch is patched by otel-cf-workers ofetch has already initialized and doesn't pick up on the new instrumented fetch object assigned to globalThis.fetch.

I think the work being done here would unblock this use-case too!

evanderkoogh/otel-cf-workers#96

@MuhammadM1998
Copy link

Related nuxt/nuxt#24519

@dwightjack dwightjack marked this pull request as ready for review July 23, 2024 05:01
Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks ❀️

@pi0 pi0 merged commit f7707b1 into unjs:main Aug 27, 2024
5 of 6 checks passed
s1gr1d added a commit to getsentry/sentry-javascript that referenced this pull request Sep 26, 2024
Test for pageload. Tests for distributed tracing for navigations are
still coming (waiting for next Nuxt version which includes the new
version of ofetch with [this
PR](unjs/ofetch#377))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants